baubs git
Commit 757e0cdd73102ce777cf51dc577f68988946f86b
Parents : 7baf03e
Author : Jan-Henrik Bruhn <jan-henrik.bruhn@offis.de>
Date : 2025-12-27T11:55:32+01:00
fix: Address PR review feedback for usePrevious implementation
- Update usePrevious hook to use useEffect pattern instead of mutating
refs during render (addresses Concurrent Mode compatibility)
- Add wasManuallyDismissed flag to properly track dismissal of all error
types (machineError, machineErrorMessage, and pyodideError)
- Add proper eslint-disable comment with explanation for ref access
- Update handlePopoverOpenChange to handle dismissal of all error types
These changes address all feedback from PR review #54
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes
2 files changed, 31 insertions(+), 23 deletions(-)
Diff
diff --git a/src/components/AppHeader.tsx b/src/components/AppHeader.tsx
index 20d9de2..c100d06 100644
--- a/src/components/AppHeader.tsx
+++ b/src/components/AppHeader.tsx
@@ -66,6 +66,7 @@ export function AppHeader() {
const [dismissedErrorCode, setDismissedErrorCode] = useState<number | null>(
null,
);
+ const [wasManuallyDismissed, setWasManuallyDismissed] = useState(false);
// Track previous values for comparison
const prevMachineError = usePrevious(machineError);
@@ -99,7 +100,7 @@ export function AppHeader() {
const hadAnyError =
prevErrorMessage || prevPyodideError || hasError(prevMachineError);
- // Auto-open popover when new error appears
+ // Auto-open popover when new error appears (but not if user manually dismissed)
const isNewMachineError =
hasError(machineError) &&
machineError !== prevMachineError &&
@@ -108,7 +109,10 @@ export function AppHeader() {
machineErrorMessage && machineErrorMessage !== prevErrorMessage;
const isNewPyodideError = pyodideError && pyodideError !== prevPyodideError;
- if (isNewMachineError || isNewErrorMessage || isNewPyodideError) {
+ if (
+ !wasManuallyDismissed &&
+ (isNewMachineError || isNewErrorMessage || isNewPyodideError)
+ ) {
setErrorPopoverOpen(true);
}
@@ -116,12 +120,14 @@ export function AppHeader() {
if (!hasAnyError && hadAnyError) {
setErrorPopoverOpen(false);
setDismissedErrorCode(null); // Reset dismissed tracking
+ setWasManuallyDismissed(false); // Reset manual dismissal flag
}
}, [
machineError,
machineErrorMessage,
pyodideError,
dismissedErrorCode,
+ wasManuallyDismissed,
prevMachineError,
prevErrorMessage,
prevPyodideError,
@@ -132,9 +138,16 @@ export function AppHeader() {
const handlePopoverOpenChange = (open: boolean) => {
setErrorPopoverOpen(open);
- // If user manually closes it, remember the current error code to prevent reopening
- if (!open && hasError(machineError)) {
- setDismissedErrorCode(machineError);
+ // If user manually closes it while any error is present, remember this to prevent reopening
+ if (
+ !open &&
+ (hasError(machineError) || machineErrorMessage || pyodideError)
+ ) {
+ setWasManuallyDismissed(true);
+ // Also track the specific machine error code if present
+ if (hasError(machineError)) {
+ setDismissedErrorCode(machineError);
+ }
}
};
diff --git a/src/hooks/usePrevious.ts b/src/hooks/usePrevious.ts
index d959bb3..26b8021 100644
--- a/src/hooks/usePrevious.ts
+++ b/src/hooks/usePrevious.ts
@@ -4,28 +4,23 @@
* Returns the previous value of a state or prop
* Useful for comparing current vs previous values in effects
*
- * Note: This uses ref access during render, which is safe for this specific use case.
- * The pattern is recommended by React for tracking previous values.
+ * This implementation updates the ref in an effect instead of during render,
+ * which is compatible with React Concurrent Mode and Strict Mode.
+ *
+ * Note: The ref read on return is safe because it's only returning the previous value
+ * (from the last render), not the current value being passed in. This is the standard
+ * pattern for usePrevious hooks.
*/
-import { useRef } from "react";
+import { useEffect, useRef } from "react";
-/* eslint-disable react-hooks/refs */
export function usePrevious<T>(value: T): T | undefined {
- const ref = useRef<{ value: T; prev: T | undefined }>({
- value,
- prev: undefined,
- });
-
- const current = ref.current.value;
+ const ref = useRef<T>();
- if (value !== current) {
- ref.current = {
- value,
- prev: current,
- };
- }
+ useEffect(() => {
+ ref.current = value;
+ }, [value]);
- return ref.current.prev;
+ // eslint-disable-next-line react-hooks/refs
+ return ref.current;
}
-/* eslint-enable react-hooks/refs */
Served by rngit 1.3.3 - Generated in 0.05s